fix(db): crash on settings restore due to schema mismatch#3693
fix(db): crash on settings restore due to schema mismatch#3693kairosci wants to merge 1 commit intoMetrolistGroup:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds DB backup-and-rollback to the restore flow in ChangesRestore with Database Backup-and-Rollback
Sequence DiagramsequenceDiagram
participant User
participant ViewModel as BackupRestoreViewModel
participant FileSystem as File System
participant SQLite as SQLite DB
participant Room as Room Instance
User->>ViewModel: Initiate restore from backup ZIP
ViewModel->>SQLite: Read current DB path/state
ViewModel->>FileSystem: Backup current DB + WAL + SHM (timestamped)
ViewModel->>FileSystem: Extract & read ZIP entries
ViewModel->>SQLite: Checkpoint & close DB
ViewModel->>FileSystem: Overwrite DB file from ZIP
ViewModel->>Room: Create new InternalDatabase instance (triggers migrations)
alt Migration Succeeds
Room->>SQLite: Migrations applied successfully
ViewModel->>FileSystem: Delete backup files
ViewModel->>User: migrationSucceeded = true -> Restart app
else Migration Fails
Room-->>ViewModel: Migration throws exception
ViewModel->>FileSystem: Restore backup DB + WAL + SHM to original path
ViewModel->>User: migrationSucceeded = false -> Show incompatible toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/di/AppModule.kt (1)
53-56: 💤 Low valueCorrectly enables fallback destructive migration to fix the restore crash.
The change appropriately allows Room to handle schema mismatches by dropping and recreating tables, which resolves the
IllegalStateExceptionwhen restoring backups from different app versions.One consideration: the
MusicDatabase.newInstance()path (seeMusicDatabase.kt:162-180) includes anonDestructiveMigrationcallback that backs up data before destruction. This DI path currently lacks that protection, so if destructive migration triggers, user data could be lost without a backup. Consider adding a similar callback for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/di/AppModule.kt` around lines 53 - 56, The DI-provided Room builder enabling fallbackToDestructiveMigration on InternalDatabase should also register an onDestructiveMigration callback to back up user data before tables are dropped; update the databaseBuilder call that returns InternalDatabase to attach an onDestructiveMigration listener (matching the behavior in MusicDatabase.newInstance / onDestructiveMigration) so it invokes the same backup routine prior to destructive migration, ensuring consistent protection of user data across both creation paths.app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt (1)
126-132: ⚡ Quick winConsider deleting WAL and SHM files after overwriting the database.
After overwriting the main database file, stale
-waland-shmfiles from the previous database may persist. These could cause corruption or inconsistent state when Room reopens the database. Thecheckpoint()call flushes WAL contents, but the files themselves may remain.🛠️ Proposed fix to clean up journal files
FileOutputStream(dbPath).use { outputStream -> inputStream.copyTo(outputStream) } + // Delete stale WAL/SHM files to prevent corruption + java.io.File("$dbPath-wal").delete() + java.io.File("$dbPath-shm").delete() Timber.tag("RESTORE").i("DB overwrite complete")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt` around lines 126 - 132, After overwriting the DB file in BackupRestoreViewModel (the block that calls runBlocking(Dispatchers.IO) { database.checkpoint() }, database.close(), and FileOutputStream(dbPath).use { ... }), delete any leftover SQLite journal files by removing files named "$dbPath-wal" and "$dbPath-shm" (check for existence and delete safely), and log success/failure with Timber.tag("RESTORE").i/e; ensure deletion happens after database.close() and after the FileOutputStream copy completes, and catch/log IO exceptions to avoid crashing the restore flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/di/AppModule.kt`:
- Around line 53-56: The DI-provided Room builder enabling
fallbackToDestructiveMigration on InternalDatabase should also register an
onDestructiveMigration callback to back up user data before tables are dropped;
update the databaseBuilder call that returns InternalDatabase to attach an
onDestructiveMigration listener (matching the behavior in
MusicDatabase.newInstance / onDestructiveMigration) so it invokes the same
backup routine prior to destructive migration, ensuring consistent protection of
user data across both creation paths.
In
`@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`:
- Around line 126-132: After overwriting the DB file in BackupRestoreViewModel
(the block that calls runBlocking(Dispatchers.IO) { database.checkpoint() },
database.close(), and FileOutputStream(dbPath).use { ... }), delete any leftover
SQLite journal files by removing files named "$dbPath-wal" and "$dbPath-shm"
(check for existence and delete safely), and log success/failure with
Timber.tag("RESTORE").i/e; ensure deletion happens after database.close() and
after the FileOutputStream copy completes, and catch/log IO exceptions to avoid
crashing the restore flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db158242-a64e-4df8-91a9-856ed5a8ed22
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/di/AppModule.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt
kairosci
force-pushed
the
fix/restore-crash-schema-mismatch
branch
from
40e5365 to
5fd44cb
Compare
kairosci
force-pushed
the
fix/restore-crash-schema-mismatch
branch
from
5fd44cb to
b6ee597
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/di/AppModule.kt (1)
51-56:⚠️ Potential issue | 🟠 MajorAdd the missing migrations to the DI provider.
The
provideInternalDatabasebuilder omits the four explicit migrations (MIGRATION_1_2,MIGRATION_21_24,MIGRATION_22_24,MIGRATION_24_25) that are registered inMusicDatabase.kt. Without them, any upgrade path not covered by those migrations will silently drop the database instead of attempting to use a migration path. Register the same migrations as the rest of the codebase:Suggested change
fun provideInternalDatabase( `@ApplicationContext` context: Context, ): InternalDatabase = Room .databaseBuilder(context, InternalDatabase::class.java, InternalDatabase.DB_NAME) + .addMigrations( + MIGRATION_1_2, + MIGRATION_21_24, + MIGRATION_22_24, + MIGRATION_24_25, + ) .fallbackToDestructiveMigration() .build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/di/AppModule.kt` around lines 51 - 56, The DI provider function provideInternalDatabase currently calls Room.databaseBuilder(...).fallbackToDestructiveMigration(dropAllTables = true) without registering explicit migrations; change it to register the same migrations used in MusicDatabase by adding .addMigrations(MIGRATION_1_2, MIGRATION_21_24, MIGRATION_22_24, MIGRATION_24_25) before build() so the InternalDatabase created by provideInternalDatabase uses those migration paths instead of always falling back to destructive migration. Ensure the migration symbols (MIGRATION_1_2, MIGRATION_21_24, MIGRATION_22_24, MIGRATION_24_25) are imported or referenced in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`:
- Around line 126-136: In BackupRestoreViewModel (the restore flow around the
code using dbPath and InternalDatabase.newInternalDatabaseInstance), delete any
existing WAL/SHM sidecar files for the database (dbPath + "-wal" and dbPath +
"-shm") before opening FileOutputStream(dbPath) to overwrite the main DB file;
follow the same pattern used in testMigration() (remove song.db-wal and
song.db-shm) so SQLite won't attempt WAL recovery against the old sidecars when
InternalDatabase (openHelper/writableDatabase) is opened later.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/di/AppModule.kt`:
- Around line 51-56: The DI provider function provideInternalDatabase currently
calls Room.databaseBuilder(...).fallbackToDestructiveMigration(dropAllTables =
true) without registering explicit migrations; change it to register the same
migrations used in MusicDatabase by adding .addMigrations(MIGRATION_1_2,
MIGRATION_21_24, MIGRATION_22_24, MIGRATION_24_25) before build() so the
InternalDatabase created by provideInternalDatabase uses those migration paths
instead of always falling back to destructive migration. Ensure the migration
symbols (MIGRATION_1_2, MIGRATION_21_24, MIGRATION_22_24, MIGRATION_24_25) are
imported or referenced in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da8a778d-94d9-41f4-b0b5-47705a5d48a3
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.ktapp/src/main/kotlin/com/metrolist/music/di/AppModule.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt
| runBlocking(Dispatchers.IO) { database.checkpoint() } | ||
| database.close() | ||
| Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath") | ||
| FileOutputStream(dbPath).use { outputStream -> | ||
| inputStream.copyTo(outputStream) | ||
| } | ||
| Timber.tag("RESTORE").i("DB overwrite complete, triggering migrations") | ||
| try { | ||
| val migratedDb = InternalDatabase.newInternalDatabaseInstance(context, InternalDatabase.DB_NAME) | ||
| migratedDb.openHelper.writableDatabase | ||
| migratedDb.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When restoring an SQLite database that uses WAL mode by replacing only the main .dbfile, should the existing-waland-shm sidecar files be removed before reopening the database?
💡 Result:
Yes—if you are restoring only by replacing the main “.db” file while leaving any existing “-wal” and “-shm” sidecar files in place, you should remove the old sidecars first (or otherwise ensure they do not exist) before reopening the database. Rationale: In WAL mode, SQLite uses the “-wal” file plus a shared-memory “-shm” wal-index/coordination file. If the existing sidecars do not match the restored “.db” state, the next opener will run WAL recovery to rebuild the wal-index from the WAL; if the WAL/index state is inconsistent with what you restored, you risk incorrect recovery or even failure. SQLite’s WAL design also notes that the “shm” content is not required for crash recovery and is normally truncated/unlinked when the database becomes quiescent, and that cleanup unlinks both “-wal” and “-shm” after a proper close. What “safe restore” usually looks like: 1) Restore/replace the main “.db”. 2) Delete the matching “-wal” and “-shm” files for that database. 3) Open the database normally; SQLite will initialize/recover the WAL-index as needed. Caveat: If some process still has the database open, deleting “-wal/-shm” can make the database become corrupted later; make sure the database is not in use during restore.
Citations:
- 1: https://www.sqlite.org/walformat.html
- 2: http://www.sqlite.com/walformat.html
- 3: https://sqlite.org/forum/forumpost/2a8c51e0b8?t=h
- 4: https://sqlite.org/forum/info/accb68153bfad38f
- 5: https://sqlite.org/forum/info/2fa9c6ac6316f7e4
- 6: https://sqlite.org/wal.html
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt | sed -n '120,145p'Repository: MetrolistGroup/Metrolist
Length of output: 2346
🏁 Script executed:
# Check if there's any cleanup of WAL/SHM files in this file
rg -A 5 -B 5 "delete|removeFile|wal|shm" app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.ktRepository: MetrolistGroup/Metrolist
Length of output: 559
🏁 Script executed:
# Check InternalDatabase for WAL mode configuration
find . -name "InternalDatabase.kt" -type f | head -1 | xargs cat -nRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Search for InternalDatabase more broadly
find . -name "*InternalDatabase*" -type fRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Search for WAL configuration in the codebase
rg -i "wal|WAL" --type kt -lRepository: MetrolistGroup/Metrolist
Length of output: 95
🏁 Script executed:
# Search for InternalDatabase class definition
rg "class InternalDatabase" --type-list | head -20Repository: MetrolistGroup/Metrolist
Length of output: 458
🏁 Script executed:
# Use correct syntax for rg
rg "class InternalDatabase" -lRepository: MetrolistGroup/Metrolist
Length of output: 128
🏁 Script executed:
# Broader search for database initialization or WAL
rg "InternalDatabase|\.database|RoomDatabase" | head -30Repository: MetrolistGroup/Metrolist
Length of output: 3836
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt | sed -n '1,100p'Repository: MetrolistGroup/Metrolist
Length of output: 4193
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt | sed -n '100,200p'Repository: MetrolistGroup/Metrolist
Length of output: 5576
🏁 Script executed:
cat -n app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt | sed -n '200,280p'Repository: MetrolistGroup/Metrolist
Length of output: 4126
Remove the old WAL/SHM sidecars before overwriting the restored database file.
backup() checkpoints and archives only the main database file, but the restore overwrites only dbPath without removing stale song.db-wal and song.db-shm files from the current install. Since the database uses WAL mode, SQLite will attempt to recover from the old WAL on the next open, which can cause integrity failures or incorrect recovery against the restored base file. The codebase already deletes these files in testMigration() (lines 254–255); apply the same pattern here before the overwrite.
Suggested change
runBlocking(Dispatchers.IO) { database.checkpoint() }
database.close()
+ java.io.File("$dbPath-wal").delete()
+ java.io.File("$dbPath-shm").delete()
Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath")
FileOutputStream(dbPath).use { outputStream ->
inputStream.copyTo(outputStream)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runBlocking(Dispatchers.IO) { database.checkpoint() } | |
| database.close() | |
| Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath") | |
| FileOutputStream(dbPath).use { outputStream -> | |
| inputStream.copyTo(outputStream) | |
| } | |
| Timber.tag("RESTORE").i("DB overwrite complete, triggering migrations") | |
| try { | |
| val migratedDb = InternalDatabase.newInternalDatabaseInstance(context, InternalDatabase.DB_NAME) | |
| migratedDb.openHelper.writableDatabase | |
| migratedDb.close() | |
| runBlocking(Dispatchers.IO) { database.checkpoint() } | |
| database.close() | |
| java.io.File("$dbPath-wal").delete() | |
| java.io.File("$dbPath-shm").delete() | |
| Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath") | |
| FileOutputStream(dbPath).use { outputStream -> | |
| inputStream.copyTo(outputStream) | |
| } | |
| Timber.tag("RESTORE").i("DB overwrite complete, triggering migrations") | |
| try { | |
| val migratedDb = InternalDatabase.newInternalDatabaseInstance(context, InternalDatabase.DB_NAME) | |
| migratedDb.openHelper.writableDatabase | |
| migratedDb.close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 126 - 136, In BackupRestoreViewModel (the restore flow around the
code using dbPath and InternalDatabase.newInternalDatabaseInstance), delete any
existing WAL/SHM sidecar files for the database (dbPath + "-wal" and dbPath +
"-shm") before opening FileOutputStream(dbPath) to overwrite the main DB file;
follow the same pattern used in testMigration() (remove song.db-wal and
song.db-shm) so SQLite won't attempt WAL recovery against the old sidecars when
InternalDatabase (openHelper/writableDatabase) is opened later.
5bfb265 to
7006b0f
Compare
Root cause: backup restore overwrites DB file but doesn't trigger Room migrations, causing schema mismatch crash on next app launch. - Remove hardcoded identity hash manipulation in BackupRestoreViewModel - Remove fallbackToDestructiveMigration from AppModule (prevents silent data loss) - Backup current DB (wal/shm files included) before restore - Trigger Room migrations by opening restored DB with proper schema - If migration succeeds: delete backup, proceed with restart - If migration fails: restore from backup, notify user, NO data loss - Add restore_database_incompatible string for user feedback - Proper control flow: only restart if migration succeeded
kairosci
force-pushed
the
fix/restore-crash-schema-mismatch
branch
from
7006b0f to
e98bef6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`:
- Around line 222-233: The current branch in BackupRestoreViewModel after
handling restore only restarts the app when migrationSucceeded != false, leaving
the process running on the incompatible-restore path and risking later DAO
access to a closed MusicDatabase; change the logic so the app always restarts
after any restore attempt (successful rollback or incompatible path) or
alternatively reinitialize the MusicDatabase singleton before returning.
Concretely, ensure the restart sequence (context.stopService(Intent(context,
MusicService::class.java)), delete persistent queue, build and start the launch
intent, and Runtime.getRuntime().exit(0)) is executed for both the
migrationSucceeded != false and the else branch (or call the existing DB
initialization routine to rebuild MusicDatabase) so no code continues running
with a closed MusicDatabase.
- Around line 107-123: The current rollback snapshot copy uses
currentDbPath/backupFile while the Room instance is still active; quiesce the DB
before taking the backup by performing the WAL checkpoint and closing the Room
database (i.e., invoke the same checkpoint/close sequence you already have
later, then compute currentDbPath and perform the File(...) copy to backupFile),
or alternatively replace the file-copy logic for creating backupFile with a call
to SQLite’s online backup API to produce a consistent snapshot; update
BackupRestoreViewModel so the code that uses currentDbPath and backupFile runs
only after the checkpoint/close (or after a successful SQLite backup call).
- Around line 169-181: The rollback restores the main DB file but only
conditionally deletes/overwrites -wal/-shm sidecars, leaving stale sidecars from
a failed migration; update the restore logic in BackupRestoreViewModel (around
backupDbFile and currentDbPath usage) to unconditionally delete any existing
currentDbPath sidecars (File("${currentDbPath}-wal") and
File("${currentDbPath}-shm")) before copying the backup and its sidecars, then
proceed to copy backup, copy its -wal/-shm if present, and delete the backups as
currently done.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a7b004a-fe78-4b6f-9957-a5f8e0c6ae21
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.ktapp/src/main/res/values/metrolist_strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/metrolist_strings.xml
| // Backup current DB before restore | ||
| val currentDbPath = database.openHelper.writableDatabase.path | ||
| val backupDbFile = if (currentDbPath != null) { | ||
| val backupFile = File("${currentDbPath}_restore_backup_${System.currentTimeMillis()}") | ||
| try { | ||
| File(currentDbPath).copyTo(backupFile, overwrite = true) | ||
| val walFile = File("${currentDbPath}-wal") | ||
| if (walFile.exists()) walFile.copyTo(File("${backupFile.absolutePath}-wal"), overwrite = true) | ||
| val shmFile = File("${currentDbPath}-shm") | ||
| if (shmFile.exists()) shmFile.copyTo(File("${backupFile.absolutePath}-shm"), overwrite = true) | ||
| Timber.tag("RESTORE").i("Created DB backup at ${backupFile.absolutePath}") | ||
| backupFile | ||
| } catch (e: Exception) { | ||
| Timber.tag("RESTORE").e(e, "Failed to create DB backup") | ||
| null | ||
| } | ||
| } else null |
There was a problem hiding this comment.
Take the rollback snapshot after the database is quiesced.
This copy happens while the current Room instance is still open and before the checkpoint/close at Lines 146-147. In WAL mode that means the backup used for rollback is not guaranteed to be a self-consistent snapshot, so a failed migration can still roll back to missing or inconsistent data. Move the checkpoint/close ahead of this copy step, or use SQLite’s backup API for the pre-restore snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 107 - 123, The current rollback snapshot copy uses
currentDbPath/backupFile while the Room instance is still active; quiesce the DB
before taking the backup by performing the WAL checkpoint and closing the Room
database (i.e., invoke the same checkpoint/close sequence you already have
later, then compute currentDbPath and perform the File(...) copy to backupFile),
or alternatively replace the file-copy logic for creating backupFile with a call
to SQLite’s online backup API to produce a consistent snapshot; update
BackupRestoreViewModel so the code that uses currentDbPath and backupFile runs
only after the checkpoint/close (or after a successful SQLite backup call).
| backupDbFile?.let { backup -> | ||
| File(currentDbPath).delete() | ||
| backup.copyTo(File(currentDbPath), overwrite = true) | ||
| backup.delete() | ||
| val walBackup = File("${backup.absolutePath}-wal") | ||
| if (walBackup.exists()) { | ||
| walBackup.copyTo(File("${currentDbPath}-wal"), overwrite = true) | ||
| walBackup.delete() | ||
| } | ||
| val shmBackup = File("${backup.absolutePath}-shm") | ||
| if (shmBackup.exists()) { | ||
| shmBackup.copyTo(File("${currentDbPath}-shm"), overwrite = true) | ||
| shmBackup.delete() |
There was a problem hiding this comment.
Clear the failed-migration sidecars before copying the backup back.
If the failed open created song.db-wal / song.db-shm and the saved backup did not include matching sidecars, those files survive this rollback and will be reopened against the restored base DB. Delete the current sidecars unconditionally before restoring the backup files.
Suggested change
backupDbFile?.let { backup ->
File(currentDbPath).delete()
+ File("${currentDbPath}-wal").delete()
+ File("${currentDbPath}-shm").delete()
backup.copyTo(File(currentDbPath), overwrite = true)
backup.delete()
val walBackup = File("${backup.absolutePath}-wal")
if (walBackup.exists()) {
walBackup.copyTo(File("${currentDbPath}-wal"), overwrite = true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 169 - 181, The rollback restores the main DB file but only
conditionally deletes/overwrites -wal/-shm sidecars, leaving stale sidecars from
a failed migration; update the restore logic in BackupRestoreViewModel (around
backupDbFile and currentDbPath usage) to unconditionally delete any existing
currentDbPath sidecars (File("${currentDbPath}-wal") and
File("${currentDbPath}-shm")) before copying the backup and its sidecars, then
proceed to copy backup, copy its -wal/-shm if present, and delete the backups as
currently done.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`:
- Around line 159-163: The code constructs wal/shm backup paths using string
interpolation on backupDbFile which can be null, producing
"null-wal"/"null-shm"; fix by guarding null before building those File
objects—e.g., only create and delete walBackup and shmBackup inside a null-check
or backupDbFile?.let { ... } block that uses the non-null reference
(backupDbFile or the lambda param) to build "${it.absolutePath}-wal" and
"${it.absolutePath}-shm" and then call exists() and delete() so no File is
created with a "null" path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 997f36f0-935a-4e65-9fad-dac9dcb1abcb
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.ktapp/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.ktapp/src/main/res/values/metrolist_strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/metrolist_strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt
| backupDbFile?.delete() | ||
| val walBackup = File("${backupDbFile?.absolutePath}-wal") | ||
| if (walBackup.exists()) walBackup.delete() | ||
| val shmBackup = File("${backupDbFile?.absolutePath}-shm") | ||
| if (shmBackup.exists()) shmBackup.delete() |
There was a problem hiding this comment.
Null-safe path construction for backup cleanup.
If backupDbFile is null, "${backupDbFile?.absolutePath}-wal" evaluates to "null-wal", creating File objects with unintended paths. While the exists() checks prevent actual harm, this is misleading.
Suggested fix
// Delete backup on success
- backupDbFile?.delete()
- val walBackup = File("${backupDbFile?.absolutePath}-wal")
- if (walBackup.exists()) walBackup.delete()
- val shmBackup = File("${backupDbFile?.absolutePath}-shm")
- if (shmBackup.exists()) shmBackup.delete()
+ backupDbFile?.let { backup ->
+ backup.delete()
+ File("${backup.absolutePath}-wal").delete()
+ File("${backup.absolutePath}-shm").delete()
+ }
migrationSucceeded = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| backupDbFile?.delete() | |
| val walBackup = File("${backupDbFile?.absolutePath}-wal") | |
| if (walBackup.exists()) walBackup.delete() | |
| val shmBackup = File("${backupDbFile?.absolutePath}-shm") | |
| if (shmBackup.exists()) shmBackup.delete() | |
| backupDbFile?.let { backup -> | |
| backup.delete() | |
| File("${backup.absolutePath}-wal").delete() | |
| File("${backup.absolutePath}-shm").delete() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 159 - 163, The code constructs wal/shm backup paths using string
interpolation on backupDbFile which can be null, producing
"null-wal"/"null-shm"; fix by guarding null before building those File
objects—e.g., only create and delete walBackup and shmBackup inside a null-check
or backupDbFile?.let { ... } block that uses the non-null reference
(backupDbFile or the lambda param) to build "${it.absolutePath}-wal" and
"${it.absolutePath}-shm" and then call exists() and delete() so no File is
created with a "null" path.
This comment was marked as spam.
This comment was marked as spam.
|
@nyxiereal @mostafaalagamy could you report this spam stuff? |
|
@kairosci they've been banned from the org |
|
Thanks! |
3 participants
Problem
Restoring a settings backup from a previous version crashes the app with
IllegalStateException: Room cannot verify the data integrity. The crash occurs because the restored database has a different schema identity hash than what the current app version expects.Root Cause
The backup restore process overwrites the database file but doesn't properly handle Room schema migrations before the app restarts. The previous approach also had hardcoded identity hash values and could silently destroy user data.
Solution (Complete fix guaranteeing data persistence)
1. Removed hardcoded identity hash manipulation
BackupRestoreViewModel.ktthat was manually inserting incorrect identity hashes intoroom_master_table2. Proper backup/restore with data safety
-waland-shmfiles)3. Removed destructive migration fallbacks
fallbackToDestructiveMigration()fromAppModule.ktand restore logic4. Added
newInternalDatabaseInstance()helperHow it works
Testing
fallbackToDestructiveMigrationremovedRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes